Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track Finding Cleanup, main branch (2025.01.07.) #808

Merged

Conversation

krasznaa
Copy link
Member

@krasznaa krasznaa commented Jan 7, 2025

As one of the final steps before finalizing #773, this PR is meant to make all the necessary changes in the common device track finding code that SYCL needs.

  • Introduced traccc::device::globalIndex_t to refer to the type of the "global index" that is given to most device functions. (Practically all the ones that don't receive a templated "thread ID" object.)
  • Updated all track finding functions to use traccc::device::globalIndex_t.
  • Removed some unnecessary templating from the code.
    • These were left over from the time when the track finding configuration was not yet the simple struct that it is today.
  • Changed some const types here and there.
    • Removed some unnecessary const statements from the "payload types". Payload objects are already given to the functions through const references. Making some (simple scalar) members of these payload objects as const has no upside in my mind.
    • Added some extra const statements on some device vectors here and there.
    • Marked the "global index" variables as const in the .ipp files. I just personally like this formalism, where in the .hpp files we don't bother with adding a const (since adding it or not makes no difference for the user of the function), but add it in the implementation for a little extra security. (Making sure that we wouldn't change these values by mistake.)
  • Tried to clean up the include statements a little. 🤔
  • Made some small updates in the CUDA track finding code to account for the changes in the common code.
    • These are all meant to be simplifications. 😉

@krasznaa krasznaa added cuda Changes related to CUDA improvement Improve an existing feature shared Changes related to shared code labels Jan 7, 2025
Introduced the globalIndex_t typedef for the unsigned int
type. Switched all track finding functions to using it
instead of std::size_t.

Removed all unnecessary templating from functions that
were never updated after the track finding configuration
became a simple standalone struct.

Removed some unnecessary const declarations from the
payload structs, and added some extra const declarations
in some of the function bodies.
@krasznaa krasznaa force-pushed the TrackFindingCleanup-main-20250107 branch from c644de5 to febca64 Compare January 7, 2025 21:07
Running too many CUDA tests in parallel requires a
*lot* of memory. This will hopefully make the tests
a bit more reliable.
@krasznaa
Copy link
Member Author

krasznaa commented Jan 8, 2025

@paulgessinger, the GitLab CI tests failure on my last commit was a weird one. After looking at it for a bit this morning, my educated guess is that the test ran out of (host) memory. 🤔

CTest was ordered to use all the cores of the test machine. Which sounds perfectly reasonable. But unfortunately this results in the CUDA driver starting to JIT compile the CUDA kernels in parallel in many processes. On my desktop with 32 GBs of memory and 20 GBs of swap I almost ran out of memory with this. 😦

For now I just reduced the number of parallel tests in the GitLab CI. 🤔 Hopefully this will not slow things down too much. This way we'll anyway make use of the CUDA driver's caching in a better way. (Once one process JIT compiles the kernel, the next process doesn't need to do it again.)

Let's see if the test now succeeds. 🤞 (I ran the flagged test through compute-sanitizer locally, but that didn't report any issues.)

@paulgessinger
Copy link
Member

@krasznaa Yeah the host memory is not a whole lot on the machine, so I can easily see it going out of memory.

Copy link

sonarqubecloud bot commented Jan 8, 2025

@krasznaa krasznaa enabled auto-merge (squash) January 8, 2025 13:28
@krasznaa krasznaa merged commit 43d3f65 into acts-project:main Jan 8, 2025
27 of 29 checks passed
@krasznaa krasznaa deleted the TrackFindingCleanup-main-20250107 branch January 8, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda Changes related to CUDA improvement Improve an existing feature shared Changes related to shared code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants